-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use log2_{v,h}_chroma_subsample #94
Conversation
ffv1.md
Outdated
PDF:`h_chroma_subsample` indicates the subsample factor between luma and chroma width ($chroma\_width=2^{-log2\_h\_chroma\_subsample}luma\_width$). | ||
RFC:`h_chroma_subsample` indicates the subsample factor between luma and chroma width (`chroma_width = 2^(-log2_h_chroma_subsample) * luma_width`). | ||
PDF:`log2_h_chroma_subsample` indicates the subsample factor, stored in powers to which the number 2 must be raised, between luma and chroma width ($chroma\_width=2^{-log2\_h\_chroma\_subsample}luma\_width$). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest avoiding lowercase must
to not confuse with RFC2119 meaning. Also stored in powers to which the number 2 must be raised
seems passive wording. Could you say the base-two logarithm of the subsample factor
...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment applies 3x below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR updated with the suggested wording.
ffv1.md
Outdated
@@ -941,8 +941,8 @@ Line( p, y ) { | | |||
`plane_pixel_width[ p ]` is the width in pixels of plane p of the slice. | |||
`plane_pixel_width[ 0 ]` and `plane_pixel_width[ 1 + ( chroma_planes ? 2 : 0 ) ]` value is `slice_pixel_width`. | |||
PDF:If `chroma_planes` is set to 1, `plane_pixel_width[ 1 ]` and `plane_pixel_width[ 2 ]` value is $\lceil slice\_pixel\_width / v\_chroma\_subsample \rceil$. | |||
RFC:If `chroma_planes` is set to 1, `plane_pixel_width[ 1 ]` and `plane_pixel_width[ 2 ]` value is `ceil(slice_pixel_width / v_chroma_subsample)`. | |||
PDF:If `chroma_planes` is set to 1, `plane_pixel_width[ 1 ]` and `plane_pixel_width[ 2 ]` value is $\lceil slice\_pixel\_width / ( 1 << log2\_h\_chroma\_subsample) \rceil$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, glad to see this fixed (v->h), though unclear why there's now a bitshift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitshift is due to the use of log2 version of the value, and means "2 power x" (see Derek comment on CELLAR)
bfef281
to
546305a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
"log2_v_chroma_subsample` indicates the subsample factor, stored in powers to which the number 2 must be raised between luma and chroma height" This sounds very odd, is this correct english ? |
Should have been changed after Dave comment, something went wrong, I force update tomorrow. |
@michaelni I check the current version of the PR, the text you are looking at is from a former version of the PR, now you should have |
73e6f51 introduces a typo: (,) |
546305a
to
563f82a
Compare
Fixed + rebased. |
Use log2_{v,h}_chroma_subsample everywhere instead of using log2_{v,h}_chroma_subsample and {v,h}_chroma_subsample depending of the context.
Issue reported by @dwbuiten
& a small typo making the spec wrong.